-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent the virtual viewport bottom being moved up unintentionally #9770
Conversation
pScreen->UpdateBottom(); | ||
|
||
// Set up text buffer | ||
pScreen->_textBuffer = std::make_unique<TextBuffer>(coordScreenBufferSize, | ||
defaultAttributes, | ||
uiCursorSize, | ||
pScreen->_renderTarget); | ||
|
||
pScreen->UpdateBottom(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had to move down, because UpdateBottom
now depends on _textBuffer
to verify that the bottom hasn't become out of range.
c83e96b
to
19f63dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fair to me and looks fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this seems fine to me too. Happy to reflect it back into windows. 😄
The only reason I had this as a draft was because I wanted to use it for a while and make sure it was working as expected. Unfortunately my home computer has now reached a point where it's more dead than alive, which means I likely won't be able to work on WT for the foreseeable future. So I'm going to mark this as ready for review now - in the time I had been using it, it seemed fine to me. |
(sorry to hear that your PC died - I will long await the days you can return in full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this looks smart to me.
Hello @miniksa! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…icrosoft#9770) ## Summary of the Pull Request The "virtual bottom" marks the last line of the mutable viewport area, which is the part of the buffer that VT sequences can write to. This region should typically only move downwards, as new lines are added to the buffer, but there were a number of cases where it was incorrectly being moved up. This PR attempts to fix that. ## PR Checklist * [x] Closes microsoft#9754 * [x] CLA signed. * [x] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: microsoft#9754 ## Detailed Description of the Pull Request / Additional comments When a call is made to `UpdateBottom`, we now clamp the value so it's at least as low as the current viewport bottom (i.e. if the viewport has moved down, we want the virtual bottom to move down too), but no lower than the bottom of the buffer (we don't want it to be out of range). There is one special case where we do actually want the virtual bottom to move up - when the scrollback has been cleared with an `ED3` escape sequence. So in that case we needed a new `ConGetSet` API (`ResetBottom`) to reset the virtual bottom to the top of the buffer (essentially one less than the viewport height, since the virtual bottom points to the last line of the viewport). ## Validation Steps Performed I had to reset the virtual bottom manually in some parts of the `ScreenBufferTests`, since some of the tests were relying on the virtual bottom being automatically reset when the viewport was reset, which is no longer the case. I've also added a new test to verify that the virtual bottom doesn't move upwards if an update is triggered while the visible viewport is scrolled up. This essentially reproduces the test case from issue microsoft#9754, which I've also manually confirmed is fixed.
The "virtual bottom" marks the last line of the mutable viewport area, which is the part of the buffer that VT sequences can write to. This region should typically only move downwards as new lines are added to the buffer, but there were a number of cases where it was incorrectly being moved up, or moved down further than necessary. This PR attempts to fix that. There was an earlier, unsuccessful attempt to fix this in PR #9770 which was later reverted (issue #9872 was the reason it had to be reverted). PRs #2666, #2705, and #5317 were fixes for related virtual viewport problems, some of which have either been extended or superseded by this PR. `SetConsoleCursorPositionImpl` is one of the cases that actually does need to move the virtual viewport upwards sometimes, in particular when the cmd shell resets the buffer with a `CLS` command. But when this operation "snaps" the viewport to the location of the cursor, it needs to use the virtual viewport as the frame of reference. This was partially addressed by PR #2705, but that only applied in terminal-scrolling mode, so I've now applied that fix regardless of the mode. `SetViewportOrigin` takes a flag which determines whether it will also move the virtual bottom to match the visible viewport. In some case this is appropriate (`SetConsoleCursorPositionImpl` being one example), but in other cases (e.g. when panning the viewport downwards in the `AdjustCursorPosition` function), it should only be allowed to move downwards. We can't just not set the update flag in those cases, because that also determines whether or not the viewport would be clamped, and we don't want change that. So what I've done is limit `SetViewportOrigin` to only move the virtual bottom downwards, and added an explicit `UpdateBottom` call in those places that may also require upward movement. `ResizeWindow` in the `ConhostInternalGetSet` class has a similar problem to `SetConsoleCursorPositionImpl`, in that it's updating the viewport to account for the new size, but if that visible viewport is scrolled back or forward, it would end up placing the virtual viewport in the wrong place. So again the solution here was to use the virtual viewport as the frame of reference for the position. However, if the viewport is being shrunk, this can still result in the cursor falling below the bottom, so we need an additional check to adjust for that. This can't be applied in pty mode, though, because that would break the conpty resizing operation. `_InternalSetViewportSize` comes into play when resizing the window manually, and again the viewport after the resize can end up truncating the virtual bottom if not handled correctly. This was partially addressed in the original code by clamping the new viewport above the virtual bottom under certain conditions, and only in terminal scrolling mode. I've replaced that with a new algorithm which links the virtual bottom to the visible viewport bottom if the two intersect, but otherwise leaves it unchanged. This applies regardless of the scrolling mode. `ResizeWithReflow` is another sizing operation that can affect the virtual bottom. This occurs when a change of the window width requires the buffer to be reflowed, and we need to reposition the viewport in the newly generated buffer. Previously we were just setting the virtual bottom to align with the new visible viewport, but that could easily result in the buffer truncation if the visible viewport was scrolled back at the time. We now set the virtual bottom to the last non-space row, or the cursor row (whichever is larger). There'll be edge cases where this is probably not ideal, but it should still work reasonably well. `MakeCursorVisible` was another case where the virtual bottom was being updated (when requested with a flag) via a `SetViewportOrigin` call. When I checked all the places it was used, though, none of them actually required that behavior, and doing so could result in the virtual bottom being incorrectly positioned, even after `SetViewportOrigin` was limited to moving the virtual bottom downwards. So I've now made it so that `MakeCursorVisible` never updates the virtual bottom. `SelectAll` in the `Selection` class was a similar case. It was calling `SetViewportOrigin` with the `updateBottom` flag set when that really wasn't necessary and could result in the virtual bottom being incorrectly set. I've changed the flag to false now. ## Validation Steps Performed I've manually confirmed that the test cases in issue #9754 are working now, except for the one involving margins, which is bigger problem with `AdjustCursorPosition` which will need to be addressed separately. I've also double checked the test cases from several other virtual bottom issues (#1206, #1222, #5302, and #9872), and confirmed that they're still working correctly with these changes. And I've added a few screen buffer tests in which I've tried to cover as many of the problematic code paths as possible. Closes #9754
Summary of the Pull Request
The "virtual bottom" marks the last line of the mutable viewport area, which is the part of the buffer that VT sequences can write to. This region should typically only move downwards, as new lines are added to the buffer, but there were a number of cases where it was incorrectly being moved up. This PR attempts to fix that.
PR Checklist
Detailed Description of the Pull Request / Additional comments
When a call is made to
UpdateBottom
, we now clamp the value so it's at least as low as the current viewport bottom (i.e. if the viewport has moved down, we want the virtual bottom to move down too), but no lower than the bottom of the buffer (we don't want it to be out of range).There is one special case where we do actually want the virtual bottom to move up - when the scrollback has been cleared with an
ED3
escape sequence. So in that case we needed a newConGetSet
API (ResetBottom
) to reset the virtual bottom to the top of the buffer (essentially one less than the viewport height, since the virtual bottom points to the last line of the viewport).Validation Steps Performed
I had to reset the virtual bottom manually in some parts of the
ScreenBufferTests
, since some of the tests were relying on the virtual bottom being automatically reset when the viewport was reset, which is no longer the case.I've also added a new test to verify that the virtual bottom doesn't move upwards if an update is triggered while the visible viewport is scrolled up. This essentially reproduces the test case from issue #9754, which I've also manually confirmed is fixed.